-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dataflow: apply diff-informed filtering consistently #17648
Conversation
@aschackmull friendly ping. |
AlertFiltering::filterByLocation(source.getLocation()) or | ||
AlertFiltering::filterByLocation(sink.getLocation()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will add a lot of inlined code and redo the filterByLocation
that was already done in isFilteredSource
/isFilteredSink
. On the other hand we don't really want to expose those two predicates beyond the SourceSinkFiltering
module, as they can be confused with isRelevantSource
/isRelevantSink
. So instead I'd suggest adding an inlined predicate to the SourceSinkFiltering
module that performs this check.
bindingset[source, sink]
pragma[inline_late]
predicate isRelevantSourceSinkPair(Node source, Node sink) {
isFilteredSource(source) or
isFilteredSink(sink)
}
and change this like so:
AlertFiltering::filterByLocation(source.getLocation()) or | |
AlertFiltering::filterByLocation(sink.getLocation()) | |
isRelevantSourceSinkPair(source.getNode(), sink.getNode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(With this change, module AlertFiltering
can remain private)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed advice! I updated the PR accordingly. PTAL.
105887f
to
1aa3eb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
No description provided.